Skip to content

Conversation

@alekhrycaiko
Copy link

Issue number:

#486

Description of changes:

This attempts to address #486.

Changes root certificate referenced based on current findings of the issue.

Testing done:

Ran test suite.

Our own cluster already relies on brupop-selfsigned-ca. But I have not ran this specific PR against the cluster and could use an additional verification/eye there.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@yeazelm yeazelm self-requested a review February 26, 2024 02:50
@yeazelm
Copy link
Contributor

yeazelm commented Feb 26, 2024

Thanks @alekhrycaiko for cutting this PR! I'm just getting up to speed on this issue so I'll need to look at this a bit more to confirm it will work. I'll dig in and get back to you! Sorry for the delay in response!

Comment on lines +199 to +207
# Source: bottlerocket-update-operator/templates/controller-priority-class.yaml
apiVersion: scheduling.k8s.io/v1
kind: PriorityClass
metadata:
name: brupop-controller-high-priority
namespace: brupop-bottlerocket-aws
preemptionPolicy: Never
value: 1000000
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running make manifest this ends up at the end rather than here. We might just move this to the end of the file to avoid it creating a dirty tree when running it, functionally it works the same.

@@ -0,0 +1,193 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this file autogenerated? I'm not sure where it came from?

pub const TLS_KEY_MOUNT_PATH: &str = "/etc/brupop-tls-keys";
// Certificate object name
pub const ROOT_CERTIFICATE_NAME: &str = "root-certificate";
pub const ROOT_CERTIFICATE_NAME: &str = "brupop-selfsigned-ca";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably safe but I'm digging into how we use cert-manager under the hood to ensure we aren't going to break something else down the line. Do you happen to have links to what led you to this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not unfortunately. I can just attest from what Jack mentions here that this resolved an issue on our end.

Copy link

@v0lumehi v0lumehi Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it might help you: I just fixed this bug and set the value to cert-manager.io/inject-ca-from: brupop-bottlerocket-aws/brupop-apiserver-certificate in the CRD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants